Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicit conversion to string in htmlspecialchars #387

Closed
wants to merge 2 commits into from

Conversation

scr4bble
Copy link

Solving PHP 8.1 deprecation warning: htmlspecialchars(): Passing null to parameter #1 ($string) of type string is deprecated

Solving PHP 8.1 deprecation warning: htmlspecialchars(): Passing null to parameter bobthecow#1 ($string) of type string is deprecated
@scr4bble
Copy link
Author

Should fix #383

@scr4bble
Copy link
Author

@bobthecow could you please give me a feedback to this PR?

@bobthecow
Copy link
Owner

Thanks for the pull request! I haven't thought through all the implications of this change, but I'm not sure it's the best answer. See my comment here: #386 (comment)

@scr4bble
Copy link
Author

@bobthecow I see. It would convert error to warning this way, if wrong variable is provided.
What about
const DEFAULT_ESCAPE = 'htmlspecialchars(%s ?? (string) %s, %s, %s)';
to just deal with nulls?

Integers should be casted automatically thanks to PHP being permissive.
Other variables should generate an error.
@scr4bble
Copy link
Author

Does it look better now?

@bobthecow
Copy link
Owner

bobthecow commented Jan 21, 2022

It … might surprise you to learn the minimum supported PHP version for this library 😛

Thinking through this a bit more, here's what basically anything you can render looks like:

PHP 5.2–7.4 … with (string) PHP 8.0+ … with (string)
null (empty string) ✅ (empty string) ✅ (empty string) ⚠️ ✅ (empty string)
false (empty string) ✅ (empty string) ✅ (empty string) ✅ (empty string)
true 1 ✅ 1 ✅ 1 ✅ 1
0 0 ✅ 0 ✅ 0 ✅ 0
42 42 ✅ 42 ✅ 42 ✅ 42
9.9 9.9 ✅ 9.9 ✅ 9.9 ✅ 9.9
'' (empty string) ✅ (empty string) ✅ (empty string) ✅ (empty string)
'foo' foo ✅ foo ✅ foo ✅ foo
Stringable class __toString return value __toString return value __toString return value __toString return value
[] (empty string, warning) ❌ Array (notice in 5.4+) 👍 (type error) ❌ Array (warning)
[1,2,3] (empty string, warning) ❌ Array (notice in 5.4+) 👍 (type error) ❌ Array (warning)
(object) [] (empty string, warning) ❌ (error) 👍 (type error) ❌ (error)
new StdClass (empty string, warning) ❌ (error) 👍 (type error) ❌ (error)
resource (empty string, warning) ❌ Resource id #1 👍 (type error) ❌ Resource id #1

We need to maintain the current (un-cast) behavior. It makes sense for a template engine.

It turns out that, with or without an explicit cast, everything behaves correctly for all scalars all the way up to PHP 8.0. PHP 8.1 looks just like PHP 8.0, but they throw in this deprecation warning when calling htmlspecialchars(null) 😕

For everything that's not a scalar, we don't want an explicit cast. Casting non-scalars to strings gives us the literal string "Array", some random warnings, notices and errors, and a "Resource id #1" thrown in for good measure. Note that PHP 8.0+ throws a type error for un-cast non-scalars, but in my opinion, that's the correct thing to do in PHP 8.0+ for all of those cases. So the un-cast version is strictly better, for every PHP version, IMO.

Which leaves us with one outlier: null in PHP 8.1+. I think we need to deal with it explicitly. And since (string) null === '' we can do this in a much more performant way!

Thank you so much for working on this. I'll push the fix I have in mind, and get it in the next release.

@bobthecow bobthecow closed this Jan 21, 2022
@scr4bble
Copy link
Author

Yes, good point about the older versions :D
Thank you for the fix! :)

@scr4bble scr4bble deleted the patch-1 branch January 21, 2022 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants